fix(genesis): normalize genesis identity comparisons - #374

Reviewed this end-to-end, including running the tests. The premise is real, the fix is correct, and the tests are genuine regression coverage. Two things need attention before merge: the fix is incomplete in one file, and CI will not execute any of it.

Verified the premise

scripts/genesis/types.ts:24 is a bare regex with no checksum enforcement and no normalizing transform:

export const schemaAddress = z.string().regex(/^0x[0-9a-fA-F]{40}$/) as z.Schema<Address>

So mixed casing is accepted and two spellings of one identity are representable. Confirmed.

Verified the tests actually catch the bug

I ran them both ways rather than taking the diff at its word:

They're real regression tests, not tautological. Good.

Incomplete: NativeFiatToken.ts minters have the identical bug

scripts/genesis/NativeFiatToken.ts:81 still uses the raw-Set pattern this PR removes everywhere else:

const minterSet = new Set()
for (const minter of data.minters) {
  if (minterSet.has(minter.address)) { /* ... */ }
  minterSet.add(minter.address)
}

I probed it against your branch with a control case:

identical casing -> success = false   (correctly rejected)
mixed casing     -> success = true    (duplicate slips through)

The impact is concrete, not theoretical. Minter storage is written via slotForAddressMap(12n, …) and slotForAddressMap(13n, …), which routes through addressToBigInt — so both spellings resolve to the same slot. A config declaring two minters with allowances A and B does not produce two minters and does not error; it produces one minter whose minterAllowed is silently whichever entry came last. That is precisely the outcome the uniqueness check exists to prevent, and minters are a privileged role.

Same one-line change you applied elsewhere:

const minterSet = new Set<string>()
for (const minter of data.minters) {
  const normalized = minter.address.toLowerCase()
  if (minterSet.has(normalized)) { /* message keeps minter.address */ }
  minterSet.add(normalized)
}

Worth adding to the security note and to the regression suite, since it's the one case with a demonstrable silent-overwrite consequence.

The call-site casts are redundant — and the fix is broader than the description says

Because the fix also landed inside enforceOperatorsNotProxyAdmin (normalizing both operands), the four .toLowerCase() as Address casts added at the ValidatorManager.ts:131 call site have no observable effect — the helper already lowercases value. I'd drop them: they add as Address casts that assert a type the expression doesn't carry, and they make the diff look narrower than it is.

That helper change actually fixes all five call sites — Denylist.ts:60, NativeFiatToken.ts:70, ProtocolConfig.ts:96, and both ValidatorManager.ts:131,175. That's a genuine strengthening and the PR description undersells it; a reviewer scanning the diff would conclude only ValidatorManager was covered.

One thing not to "fix" while you're in there: Denylist's denylisters array has no uniqueness check at all, so casing is moot for it. Adding normalization there without adding the uniqueness check would be cosmetic.

CI will not run these tests, and will not lint these files

This matters for your own "run these before merging" note. The target exists and does cover the file — Makefile:176:

test-unit-hardhat: ## Run hardhat unit tests
	npx hardhat test ./tests/helpers/matchers/index.test.ts ./tests/unit/*.test.ts --no-compile

But nothing invokes it:

• .github/workflows/ci.yml is 288 lines and the only make invocation in the entire .github/workflows/ directory is make up. There is no npx hardhat test and no reference to tests/unit anywhere in it.
• make test-unit does not chain to it either — it runs make lint plus cargo nextest.
• The eslint step explicitly excludes both changed paths: npx eslint --ignore-pattern 'scripts/' --ignore-pattern 'tests/'.

So a green CI run on this PR does not mean the new regression tests passed — they will not have executed, and neither changed file is in lint scope. Since fork PRs need maintainer CI approval anyway, I'd state the local results explicitly in the PR body. The results above are reproducible with npx hardhat test ./tests/unit/validator-manager-genesis-validation.test.ts --no-compile after npm ci.

A stronger root-cause option, with evidence that it's free

Normalizing at comparison boundaries is the right tactical fix, but it's a discipline the codebase now has to maintain forever — the NativeFiatToken miss above is that discipline failing on the very first pass. Two root-cause options:

1. schemaAddress.transform(s => s.toLowerCase()) — fixes every present and future comparison at once, but changes serialization. I'd avoid it: committed genesis artifacts carry mixed-case addresses (34 of 294 in assets/testnet/genesis.json) and the testnet genesis hash is pinned by test, so this needs hash re-verification.

2. Enforce EIP-55 checksum in schemaAddress. Strictly stronger than normalization: with checksum enforced there is exactly one valid spelling per address, so casing-duplicates become unrepresentable rather than merely detected. And it is serialization-neutral — I checksummed every address in the committed configs:

   file	unique addresses	valid EIP-55
   assets/devnet/config.json	26	26
   assets/mainnet/config.json	40	40
   assets/testnet/config.json	26	26

   All 92 already pass, so enforcing it changes no committed config and no output. It also catches single-character typos, which normalization silently accepts.

I'd keep this PR as the safe, reviewable fix and treat checksum enforcement as a separate change — but it's worth recording as the durable version, because normalization-at-comparison relies on every future author remembering.

Minor

• security/ is a new top-level directory that doesn't exist on main. Worth confirming maintainers want that location rather than docs/.
• The security note ends with "Before upstream submission, run the repository's normal TypeScript formatting, linting, and unit-test commands" — that's a working note to yourself, and reads oddly in a committed file since this is the upstream submission.
• overrides: Record<string, unknown> in the test helper drops type-checking on the override, so a typo'd key would silently produce a config that passes for the wrong reason. Typing it as Partial<ReturnType<typeof configWithValidators>> keeps the tests honest.
• 9 commits including staging/unstaging churn (stage EIP-7702 research, remove unrelated finding, separate EIP-7702 research). Worth squashing so the history is just the fix, tests, and doc.

For what it's worth, AccountCreator.ts is clean — its Map/Set keys are numeric registration IDs, not hex, so it isn't affected.

Disclosure: I'm an external community contributor, not affiliated with Circle, with no write access to this repository. Advisory only. Test results above were produced locally against PR head c7189f3 and main at de76122; the EIP-55 check used a self-tested keccak256 implementation verified against the standard empty-string and abc vectors.

Validation update
Re-ran the validation locally after initializing the repository submodules:
Focused genesis regression tests: 14 passing
Full Hardhat unit suite (make test-unit-hardhat): 47 passing
make lint: passed
git diff --check: clean
Foundry: v1.4.4
The working tree is clean and no dependency/package changes are included.
Also verified the mixed-case NativeFiatToken minter regression coverage alongside the ValidatorManager cases.
The change remains scoped to genesis identity normalization and regression coverage.

Reviewed this end-to-end, including running the tests. The premise is real, the fix is correct, and the tests are genuine regression coverage. Two things need attention before merge: the fix is incomplete in one file, and CI will not execute any of it.

Verified the premise

scripts/genesis/types.ts:24 is a bare regex with no checksum enforcement and no normalizing transform:

export const schemaAddress = z.string().regex(/^0x[0-9a-fA-F]{40}$/) as z.Schema<Address>

So mixed casing is accepted and two spellings of one identity are representable. Confirmed.

Verified the tests actually catch the bug

I ran them both ways rather than taking the diff at its word:

tree result
PR head c7189f3 9 passing
main + only the PR's test file 5 failing — exactly the 5 new cases
They're real regression tests, not tautological. Good.

Incomplete: NativeFiatToken.ts minters have the identical bug

scripts/genesis/NativeFiatToken.ts:81 still uses the raw-Set pattern this PR removes everywhere else:

const minterSet = new Set()
for (const minter of data.minters) {
  if (minterSet.has(minter.address)) { /* ... */ }
  minterSet.add(minter.address)
}

I probed it against your branch with a control case:

identical casing -> success = false   (correctly rejected)
mixed casing     -> success = true    (duplicate slips through)

The impact is concrete, not theoretical. Minter storage is written via slotForAddressMap(12n, …) and slotForAddressMap(13n, …), which routes through addressToBigInt — so both spellings resolve to the same slot. A config declaring two minters with allowances A and B does not produce two minters and does not error; it produces one minter whose minterAllowed is silently whichever entry came last. That is precisely the outcome the uniqueness check exists to prevent, and minters are a privileged role.

Same one-line change you applied elsewhere:

const minterSet = new Set<string>()
for (const minter of data.minters) {
  const normalized = minter.address.toLowerCase()
  if (minterSet.has(normalized)) { /* message keeps minter.address */ }
  minterSet.add(normalized)
}

Worth adding to the security note and to the regression suite, since it's the one case with a demonstrable silent-overwrite consequence.

The call-site casts are redundant — and the fix is broader than the description says

Because the fix also landed inside enforceOperatorsNotProxyAdmin (normalizing both operands), the four .toLowerCase() as Address casts added at the ValidatorManager.ts:131 call site have no observable effect — the helper already lowercases value. I'd drop them: they add as Address casts that assert a type the expression doesn't carry, and they make the diff look narrower than it is.

That helper change actually fixes all five call sites — Denylist.ts:60, NativeFiatToken.ts:70, ProtocolConfig.ts:96, and both ValidatorManager.ts:131,175. That's a genuine strengthening and the PR description undersells it; a reviewer scanning the diff would conclude only ValidatorManager was covered.

One thing not to "fix" while you're in there: Denylist's denylisters array has no uniqueness check at all, so casing is moot for it. Adding normalization there without adding the uniqueness check would be cosmetic.

CI will not run these tests, and will not lint these files

This matters for your own "run these before merging" note. The target exists and does cover the file — Makefile:176:

test-unit-hardhat: ## Run hardhat unit tests
	npx hardhat test ./tests/helpers/matchers/index.test.ts ./tests/unit/*.test.ts --no-compile

But nothing invokes it:

• .github/workflows/ci.yml is 288 lines and the only make invocation in the entire .github/workflows/ directory is make up. There is no npx hardhat test and no reference to tests/unit anywhere in it.
• make test-unit does not chain to it either — it runs make lint plus cargo nextest.
• The eslint step explicitly excludes both changed paths: npx eslint --ignore-pattern 'scripts/' --ignore-pattern 'tests/'.

So a green CI run on this PR does not mean the new regression tests passed — they will not have executed, and neither changed file is in lint scope. Since fork PRs need maintainer CI approval anyway, I'd state the local results explicitly in the PR body. The results above are reproducible with npx hardhat test ./tests/unit/validator-manager-genesis-validation.test.ts --no-compile after npm ci.

A stronger root-cause option, with evidence that it's free

Normalizing at comparison boundaries is the right tactical fix, but it's a discipline the codebase now has to maintain forever — the NativeFiatToken miss above is that discipline failing on the very first pass. Two root-cause options:

1. schemaAddress.transform(s => s.toLowerCase()) — fixes every present and future comparison at once, but changes serialization. I'd avoid it: committed genesis artifacts carry mixed-case addresses (34 of 294 in assets/testnet/genesis.json) and the testnet genesis hash is pinned by test, so this needs hash re-verification.

2. Enforce EIP-55 checksum in schemaAddress. Strictly stronger than normalization: with checksum enforced there is exactly one valid spelling per address, so casing-duplicates become unrepresentable rather than merely detected. And it is serialization-neutral — I checksummed every address in the committed configs:

   file
   unique addresses
   valid EIP-55

   assets/devnet/config.json
   26
   26

   assets/mainnet/config.json
   40
   40

   assets/testnet/config.json
   26
   26

   All 92 already pass, so enforcing it changes no committed config and no output. It also catches single-character typos, which normalization silently accepts.

I'd keep this PR as the safe, reviewable fix and treat checksum enforcement as a separate change — but it's worth recording as the durable version, because normalization-at-comparison relies on every future author remembering.

Minor

• security/ is a new top-level directory that doesn't exist on main. Worth confirming maintainers want that location rather than docs/.
• The security note ends with "Before upstream submission, run the repository's normal TypeScript formatting, linting, and unit-test commands" — that's a working note to yourself, and reads oddly in a committed file since this is the upstream submission.
• overrides: Record<string, unknown> in the test helper drops type-checking on the override, so a typo'd key would silently produce a config that passes for the wrong reason. Typing it as Partial<ReturnType<typeof configWithValidators>> keeps the tests honest.
• 9 commits including staging/unstaging churn (stage EIP-7702 research, remove unrelated finding, separate EIP-7702 research). Worth squashing so the history is just the fix, tests, and doc.

For what it's worth, AccountCreator.ts is clean — its Map/Set keys are numeric registration IDs, not hex, so it isn't affected.

Disclosure: I'm an external community contributor, not affiliated with Circle, with no write access to this repository. Advisory only. Test results above were produced locally against PR head c7189f3 and main at de76122; the EIP-55 check used a self-tested keccak256 implementation verified against the standard empty-string and abc vectors.

Thanks for the detailed review. I addressed the NativeFiatToken minter finding as suggested.
Normalized minter addresses at the uniqueness-check boundary using toLowerCase().
Added regression coverage for both identical-case and mixed-case duplicate minters.
Added a positive case confirming distinct minter addresses remain valid.
Updated the security note to document the concrete storage-slot collision / last-write-wins impact.
I also removed the redundant .toLowerCase() as Address call-site casts since enforceOperatorsNotProxyAdmin() now normalizes both operands internally.
I kept EIP-55 checksum enforcement out of this PR as suggested, since that would be a broader input-validation policy change.
After the changes, I re-ran the validation:
Focused genesis regression tests: 14 passing
Full Hardhat unit suite: 47 passing
make lint: passed
git diff --check: clean
Thanks again for catching the NativeFiatToken case — it made the fix materially more complete.

Re-verified end-to-end at dea806c. The minter fix is correct and is now genuine regression coverage, and every minor point is addressed. One new test does not do what it looks like it does, and there is a related trap worth flagging before someone "completes" the fix.

Confirmed

Ran the PR's test files against main's scripts again, same method as before. It was 5 failing; it is now 7 failing / 7 passing, and the two newly-failing cases are exactly the ones you added:

• rejects duplicate minters when address casing differs — fails on main, passes here
• rejects a role colliding with the proxy admin when address casing differs (NativeFiatToken) — same

So the minter normalization is pinned by a test that genuinely fails without it. Counts reproduce: 14 focused, 47 full Hardhat suite, both exit 0. git diff --check clean. Squashed to 1 commit (was 9), doc relocated to docs/security/, the "Before upstream submission" working note is gone, and overrides is now Partial<ValidatorManagerConfig> — cleaner than what I suggested. I could not run make lint (its first target shells out to cargo, which is not on PATH in my environment), so I am taking that claim as unverified rather than confirmed.

The proxy-address test is vacuous

accepts a ValidatorRegistry proxy address when only hexadecimal casing differs passes on unmodified main — it is one of the 7 that do not fail. The reason is in the fixture:

const proxyAddress = '0x3600000000000000000000000000000000000002'
proxyAddress.toUpperCase().replace('0X', '0x')  // -> identical string

That address contains no alphabetic hex digits (only 3, 6, 0, 2), so upper-casing it is a no-op and the "casing variant" is byte-identical to the original. The test asserts that an unmodified address parses — true before and after the change.

Corollary: that production line is also a no-op — and so is the "inconsistency" around it

The same fact makes the normalization itself unreachable:

data.proxy.address.toLowerCase() !== DEFAULT_VALIDATOR_REGISTRY_PROXY_ADDRESS.toLowerCase()

No mixed-case spelling of an all-numeric address exists, so this can never differ from the raw comparison. Harmless to keep as defensive, but it is not fixing a reachable case.

The trap: two comparisons of identical shape were left un-normalized, and they look like the fix being incomplete —

• NativeFiatToken.ts:108 — proxy.address !== FIAT_TOKEN_ADDRESS
• ValidatorManager.ts:272 — validatorRegistryAddress !== DEFAULT_VALIDATOR_REGISTRY_PROXY_ADDRESS

They are not defects. All four system-contract constants in addresses.ts (0x36..00 through 0x36..03) are all-numeric, so casing cannot vary for any of them either. I checked before reporting them as a gap, which is what turned up the vacuity above. Worth stating explicitly in the thread so a later reviewer does not "complete" the fix by normalizing two more lines that cannot matter — and so the asymmetry does not read as an oversight.

The distinction that matters: the checks worth normalizing compare two config-supplied values (minter vs minter, operator vs proxy admin, controller vs controller) — both sides free-form, both able to vary in case. Comparisons against a hardcoded system constant have one fixed side, and here that side has no letters. Your six real fixes are all in the first category; only the proxy-address one is in the second.

Two small consequences:

• The security note lists ValidatorRegistry proxy-address comparison under Affected validation paths. It is not affected — no input can trigger it. Worth dropping or annotating, since the doc is the durable artifact.
• The vacuous test is harmless but misleading. Either drop it, or make it meaningful by pointing it at an address that actually has alphabetic digits (denylistAddressByNetwork.testnet is mixed-case, for instance) if there is a comparison path where that applies.

Everything else stands — the fix is materially more complete than the previous round, and the minter case was the one with a real silent-overwrite consequence.

Disclosure: I'm an external community contributor, not affiliated with Circle, with no write access to this repository. Advisory only. Results above are local, against PR head dea806c and main at de76122; CI still has not run here, since external fork PRs sit behind the workflow-approval gate.

Thanks for the thorough independent review and for verifying the regression behavior against both the PR and main.

I’ve addressed the identified gaps:

Added case-insensitive minter uniqueness validation and regression coverage.
Removed the vacuous ValidatorRegistry proxy-address casing test.

Updated the security documentation to reflect the actual affected validation paths.
Updated the PR description with the regression coverage and CI limitations.
The remaining mixed-case identity collision cases are covered, while the EIP-55 checksum idea is intentionally kept separate from this PR.